Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(node): Run Node CI tests on Windows #4616

Closed
wants to merge 18 commits into from

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Feb 22, 2022

Closes #4613

@timfish timfish force-pushed the test/node-windows branch 3 times, most recently from fa27a25 to eaab502 Compare February 22, 2022 20:22
@AbhiPrasad
Copy link
Member

After these changes are in a good place, we'll have to update the CI to make these required.

@AbhiPrasad AbhiPrasad added this to the Pre 7.0.0 Work milestone Feb 23, 2022
@timfish timfish mentioned this pull request Feb 24, 2022
15 tasks
@timfish
Copy link
Collaborator Author

timfish commented Feb 24, 2022

Still trying to work out why the build cache it not being found from the windows tests:
https://github.com/getsentry/sentry-javascript/runs/5318935426?check_suite_focus=true#step:5:24

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Feb 24, 2022

diff --git a/packages/utils/test/is.test.ts b/packages/utils/test/is.test.ts
index 0b8f1a9d..5b06b7ad 100644
--- a/packages/utils/test/is.test.ts
+++ b/packages/utils/test/is.test.ts
@@ -1,6 +1,6 @@
-import { resolvedSyncPromise } from '../dist';
 import { isDOMError, isDOMException, isError, isErrorEvent, isInstanceOf, isPrimitive, isThenable } from '../src/is';
 import { supportsDOMError, supportsDOMException, supportsErrorEvent } from '../src/supports';
+import { resolvedSyncPromise } from '../src/syncpromise';
 
 class SentryError extends Error {
   public name: string;
diff --git a/packages/utils/test/syncpromise.test.ts b/packages/utils/test/syncpromise.test.ts
index 7c693c82..6164c666 100644
--- a/packages/utils/test/syncpromise.test.ts
+++ b/packages/utils/test/syncpromise.test.ts
@@ -1,5 +1,4 @@
-import { rejectedSyncPromise, resolvedSyncPromise } from '../dist';
-import { SyncPromise } from '../src/syncpromise';
+import { rejectedSyncPromise, resolvedSyncPromise, SyncPromise } from '../src/syncpromise';
 
 describe('SyncPromise', () => {
   test('simple', () => {

Needs to be applied, importing from the dist files is a code smell - I'll open the PR

As for the build.test.ts 🤔

Could we debug by running GHA locally with https://github.com/nektos/act?

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 1, 2022

Needs to be applied, importing from the dist files is a code smell - I'll open the PR

Made this change in #4631

build.test.ts

I'd be fine with moving https://github.com/getsentry/sentry-javascript/blob/master/packages/utils/test/build.test.ts into a node script that runs in the job_browser_build_tests

We shouldn't be asserting on built files in jest unit tests.

@timfish
Copy link
Collaborator Author

timfish commented Mar 1, 2022

Thanks, will take another look!

AbhiPrasad added a commit that referenced this pull request Mar 1, 2022
We shouldn't be asserting on built assets (dist/esm) in our unit tests.

This helps unblock #4616
AbhiPrasad added a commit that referenced this pull request Mar 1, 2022
We shouldn't be asserting on built assets (dist/esm) in our unit tests.

This helps unblock #4616
@AbhiPrasad
Copy link
Member

Opened #4658 for the build unit tests

@timfish
Copy link
Collaborator Author

timfish commented Mar 2, 2022

I managed to work around the fact that build caches cannot be shared between platforms by instead uploading and downloading artefacts.

However, sharing an install cache between Windows agents does not work because the yarn workspace symlinks do not get copied. I think this is because unlike unix platforms, symlinks on Windows are not stored in the regular file system 🤦‍♂️.

This means that on Windows we have to do a full yarn install in every agent and this is very very slow.

I'm not convinced it's a great idea to add this into the CI build yet!

@github-actions
Copy link
Contributor

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Also run node.js unit/e2e tests on Windows
2 participants